[vpp] Enable copy pass thru for VAAPI #1268

Alexandr-Konovalov · 2019-03-15T09:26:11Z

Issue: MDP-53850
Fixes: #1027

mgonchar · 2019-03-15T09:40:23Z

_studio/mfx_lib/vpp/include/mfx_vpp_hw.h

@@ -908,6 +908,8 @@ namespace MfxHwVideoProcessing
            mfxFrameSurface1 *pInputSurface,
            mfxFrameSurface1 *pOutputSurface);

+        mfxStatus IsCopyPassThroughPreferred(bool &preferred) const;


This design looks ugly, this function should simply return bool

I don't like this function:). Both prototype (+1 to @mgonchar) and how it named/what it does. It returns not deterministic result: "I check copy pass thru, maybe I like it maybe not - decide later". I would like to see function named useSOMETHING() with the obvious inclination.

To be honest, I got lost in CopyPassThrough name. What it actually mean? Copy with memcpy or CM?

I tried to say something like "performance of CopyPassThrough is good, you want to use it" vs "it's better to omit CopyPassThrough" and to hide low-level details what is used inside and what is faster.

mgonchar · 2019-03-15T09:48:38Z

_studio/mfx_lib/vpp/src/mfx_vpp_hw.cpp

@@ -3115,9 +3134,16 @@ mfxStatus VideoVPPHW::VppFrameCheck(
 #endif
    MFX_CHECK_STS(sts);

+    bool useCopyPassThrough = false;
+    if (true == m_config.m_bCopyPassThroughEnable && false == IsRoiDifferent(pTask->input.pSurf, pTask->output.pSurf))


I don't like bool to bool conversions.
As for me

if (m_config.m_bCopyPassThroughEnable && !IsRoiDifferent(pTask->input.pSurf, pTask->output.pSurf))

is much better.
But I won't insist of changing this if you like your approach

This is legacy. But I'm for the change, sure.

mgonchar · 2019-03-15T09:53:18Z

_studio/mfx_lib/vpp/src/mfx_vpp_hw.cpp

@@ -2962,6 +2962,25 @@ mfxStatus VideoVPPHW::Close()
 } // mfxStatus VideoVPPHW::Close()


+mfxStatus VideoVPPHW::IsCopyPassThroughPreferred(bool &preferred) const
+{
+    if (m_pCore->GetVAType() != MFX_HW_VAAPI || m_ioMode != D3D_TO_D3D)


Doesn't this check mean that vaapi core is used?
I.e. cast on line 2979 wouldn't fail?

@mgonchar Yes, the cast must not fail. I don't like dropping the check, however. What can I do here, as there is no asserts in release and you think returning mfxStatus is ugly?

well, currently I see two different ways:

Leave the current code, but return false instead of error if cast fails (which is supposed to be almost impossible situation). So we just silently hide the error (not actually so, because we can use trace macro to at least signal to stdout if traces are on)

Move to references casting, then some upper layers should deal with possible exception (which is supposed to be almost impossible situation). So we loose information about place of error occurrence.

So both of these ways are definitely not much better than current approach. I will try to figure out better solution...

Asserts are enabled in release builds of master branch. They are switched off in builds of release branches. So, use assert if you feel it right.

Here I want to be protected from some fearless code refactoring. So checking on master' release is just fine.

Having virtual GetCmCopyStatus in core is not an option to avoid this cast which is ugly on its own?

I believe adding new virtual functions to CommonCORE is prohibited. That's why the cast was applied here.

dvrogozh · 2019-03-15T17:35:42Z

_studio/mfx_lib/vpp/include/mfx_vpp_hw.h

@@ -908,6 +908,8 @@ namespace MfxHwVideoProcessing
            mfxFrameSurface1 *pInputSurface,
            mfxFrameSurface1 *pOutputSurface);

+        mfxStatus IsCopyPassThroughPreferred(bool &preferred) const;


I don't like this function:). Both prototype (+1 to @mgonchar) and how it named/what it does. It returns not deterministic result: "I check copy pass thru, maybe I like it maybe not - decide later". I would like to see function named useSOMETHING() with the obvious inclination.

To be honest, I got lost in CopyPassThrough name. What it actually mean? Copy with memcpy or CM?

dvrogozh · 2019-03-15T17:37:12Z

_studio/mfx_lib/vpp/src/mfx_vpp_hw.cpp

@@ -2962,6 +2962,25 @@ mfxStatus VideoVPPHW::Close()
 } // mfxStatus VideoVPPHW::Close()


+mfxStatus VideoVPPHW::IsCopyPassThroughPreferred(bool &preferred) const
+{
+    if (m_pCore->GetVAType() != MFX_HW_VAAPI || m_ioMode != D3D_TO_D3D)


Asserts are enabled in release builds of master branch. They are switched off in builds of release branches. So, use assert if you feel it right.

dvrogozh · 2019-03-15T17:49:21Z

_studio/shared/include/libmfx_core_vaapi.h

@@ -177,6 +177,8 @@ class VAAPIVideoCORE : public CommonCORE
    // this function should not be virtual
    mfxStatus SetCmCopyStatus(bool enable);

+    bool GetCmCopyStatus() const { return m_bCmCopy; }


Status in function name is misleading. Can we just have SetCmCopy(bool) and bool CmCopy()?

mgonchar · 2019-03-18T10:15:25Z

_studio/shared/src/libmfx_core_vaapi.cpp

@@ -813,7 +813,7 @@ VAAPIVideoCORE::GetVAService(
 } // mfxStatus VAAPIVideoCORE::GetVAService(...)

 mfxStatus
-VAAPIVideoCORE::SetCmCopyStatus(bool enable)
+VAAPIVideoCORE::SetCmCopy(bool enable)


I believe no sense for this function to return status

@mgonchar Is it possible to change mfxStatus CMEnabledCoreInterface::SetCmCopy(bool enable) to return void as well? Can it affect ABI of some plugins?

@onabiull : please, comment.

If we wan't old plugins to remain functional, we need change interfaces by adding new ones. This would affect windows version in the first hand.

@onabiull : Does this apply to VAAPIVideoCORE or VideoCORE or both?

If I understand Oleg correctly, CMEnabledCoreInterface is part of plugin API and so change of return value's type is prohibited.

So, rename virtual mfxStatus CMEnabledCoreInterface::SetCmCopyStatus(bool enable) to virtual mfxStatus CMEnabledCoreInterface::SetCmCopy(bool enable) is also incorrect and must be reverted.

dvrogozh · 2019-03-18T14:31:14Z

_studio/mfx_lib/vpp/src/mfx_vpp_hw.cpp

+{
+    if (m_pCore->GetVAType() != MFX_HW_VAAPI || m_ioMode != D3D_TO_D3D)
+    {
+        return true; // preferred unconditionally


after function rename this comment is misleading

dvrogozh · 2019-03-18T14:35:13Z

_studio/mfx_lib/vpp/src/mfx_vpp_hw.cpp

@@ -3115,9 +3131,13 @@ mfxStatus VideoVPPHW::VppFrameCheck(
 #endif
    MFX_CHECK_STS(sts);

+    bool useCopyPassThrough =
+        (m_config.m_bCopyPassThroughEnable && !IsRoiDifferent(pTask->input.pSurf, pTask->output.pSurf))?


Why you don't want to move this condition to UseCopyPassThrough() as well?

dvrogozh · 2019-03-18T14:36:17Z

_studio/shared/src/libmfx_core_vaapi.cpp

@@ -813,7 +813,7 @@ VAAPIVideoCORE::GetVAService(
 } // mfxStatus VAAPIVideoCORE::GetVAService(...)

 mfxStatus
-VAAPIVideoCORE::SetCmCopyStatus(bool enable)
+VAAPIVideoCORE::SetCmCopy(bool enable)


@onabiull : please, comment.

dvrogozh · 2019-03-18T18:13:08Z

_studio/mfx_lib/vpp/src/mfx_vpp_hw.cpp

+    // This can't be done in ConfigureExecuteParams() because CmCopy status is set later.
+    const VAAPIVideoCORE *vaapiCore = dynamic_cast<VAAPIVideoCORE*>(m_pCore);
+    assert(vaapiCore);
+    return vaapiCore->CmCopy();


you know, I am not sure I like this code. I would probably go with just:

const VAAPIVideoCORE *vaapiCore = dynamic_cast<VAAPIVideoCORE*>(m_pCore); return (vaapiCore)? vaapiCore->CmCopy(): false;

There is a huge "Can't happen!" and we are ignoring it silently, aren't we? Why not add
if(!vaapiCore) MFX_STS_TRACE(); at least?

dvrogozh · 2019-03-18T18:13:59Z

_studio/mfx_lib/vpp/src/mfx_vpp_hw.cpp

@@ -2962,6 +2962,28 @@ mfxStatus VideoVPPHW::Close()
 } // mfxStatus VideoVPPHW::Close()


+bool VideoVPPHW::UseCopyPassThrough(const DdiTask *pTask) const


the name of the function should probably be: UseCmCopyPassThrough(), right?

The plan was to hide mere presence of Cm. Why may a user of CopyPassThrought be interested in what is used inside? One just want fastest version, I think.

Because it seems that this function implies that CM is going to be used. At least that's what I think looking into last line in its code.

Ok, I got it after offline discussion. Under copy pass through we understand any mem copy operation without VP algorithms. This can be map+memcpy or cmcopy. In different modes we prefer different copy type.

dvrogozh · 2019-03-18T18:15:15Z

_studio/shared/src/libmfx_core_vaapi.cpp

@@ -813,7 +813,7 @@ VAAPIVideoCORE::GetVAService(
 } // mfxStatus VAAPIVideoCORE::GetVAService(...)

 mfxStatus
-VAAPIVideoCORE::SetCmCopyStatus(bool enable)
+VAAPIVideoCORE::SetCmCopy(bool enable)


@onabiull : Does this apply to VAAPIVideoCORE or VideoCORE or both?

dvrogozh · 2019-03-19T15:05:46Z

_studio/mfx_lib/vpp/src/mfx_vpp_hw.cpp

@@ -2962,6 +2962,28 @@ mfxStatus VideoVPPHW::Close()
 } // mfxStatus VideoVPPHW::Close()


+bool VideoVPPHW::UseCopyPassThrough(const DdiTask *pTask) const


Ok, I got it after offline discussion. Under copy pass through we understand any mem copy operation without VP algorithms. This can be map+memcpy or cmcopy. In different modes we prefer different copy type.

mgonchar · 2019-03-19T18:45:09Z

_studio/mfx_lib/vpp/src/mfx_vpp_hw.cpp

+    // for CM availability to do fallback to VPP if there is no CM.
+    // This can't be done in ConfigureExecuteParams() because CmCopy status is set later.
+    const VAAPIVideoCORE *vaapiCore = dynamic_cast<VAAPIVideoCORE*>(m_pCore);
+    assert(vaapiCore);


I think here you can use following macro to simplify code + use assert + use traces:

MFX_CHECK_WITH_ASSERT(vaapiCore, false); return vaapiCore->CmCopy();

Issue: MDP-53850 Fixes: Intel-Media-SDK#1027

Alexandr-Konovalov requested review from alariono, artem-shaporenko, dmitryermilov, dvrogozh, onabiull, snowpiel, varistar and a team as code owners March 15, 2019 09:26

mgonchar reviewed Mar 15, 2019

View reviewed changes

dvrogozh suggested changes Mar 15, 2019

View reviewed changes

Alexandr-Konovalov force-pushed the copy-pass-through branch from 3ca8c40 to 4113007 Compare March 18, 2019 08:43

mgonchar reviewed Mar 18, 2019

View reviewed changes

dvrogozh suggested changes Mar 18, 2019

View reviewed changes

Alexandr-Konovalov force-pushed the copy-pass-through branch from 4113007 to 3bbf791 Compare March 18, 2019 16:41

dvrogozh suggested changes Mar 18, 2019

View reviewed changes

dvrogozh suggested changes Mar 19, 2019

View reviewed changes

Alexandr-Konovalov force-pushed the copy-pass-through branch from 3bbf791 to 039ecba Compare March 19, 2019 16:38

mgonchar reviewed Mar 19, 2019

View reviewed changes

Alexandr-Konovalov force-pushed the copy-pass-through branch 2 times, most recently from d742fa8 to c5495f7 Compare March 20, 2019 07:27

[vpp] Enable copy pass thru for VAAPI

0e11d31

Issue: MDP-53850 Fixes: Intel-Media-SDK#1027

Alexandr-Konovalov force-pushed the copy-pass-through branch from c5495f7 to 0e11d31 Compare March 20, 2019 07:29

mgonchar approved these changes Mar 21, 2019

View reviewed changes

dvrogozh approved these changes Mar 21, 2019

View reviewed changes

dvrogozh added the pr-ready Pull request is ready for merge label Mar 21, 2019

onabiull approved these changes Mar 21, 2019

View reviewed changes

onabiull merged commit 5d89dc0 into Intel-Media-SDK:master Mar 21, 2019

Alexandr-Konovalov deleted the copy-pass-through branch March 21, 2019 15:25

fulinjie mentioned this pull request Jun 18, 2020

Pass through mode for VIDEO->SYSTEM with MFX_FOURCC_A2RGB10 format #1654

Open

		@@ -2962,6 +2962,28 @@ mfxStatus VideoVPPHW::Close()
		} // mfxStatus VideoVPPHW::Close()


		bool VideoVPPHW::UseCopyPassThrough(const DdiTask *pTask) const

[vpp] Enable copy pass thru for VAAPI #1268

[vpp] Enable copy pass thru for VAAPI #1268

Conversation

Alexandr-Konovalov commented Mar 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment